-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement/577 Threshold values for Embeddings #621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's some PHPCS warnings that we should look to fix: https://github.com/10up/classifai/actions/runs/6968812438/job/18963465180#step:8:16. Not sure why it's not showing this check as having failed, maybe we only fail on errors and not warnings? But these are easy enough to fix up so ideally we address these
EDIT: looked into this and seems it's due to the 10up-Default ruleset we extend, based on this line: https://github.com/10up/phpcs-composer/blob/master/10up-Default/ruleset.xml#L33
Personally I think we should remove this so we get failures on warnings, otherwise those items never get fixed up.
@dkotter thanks for the feedback. I've implemented them (and also fixed the minor phpcs warnings). Please check.
Agree. Should I raise a PR there? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In testing, found a few things:
- When first going to the settings page, I don't see any default values for the thresholds. After saving the first time, they get set to 75% but those should be set on initial load:
- If the Watson feature is turned off, doesn't seem we load the language-processing.css file, so styles aren't getting loaded:
- Don't think this is a side-effect of this PR (probably due to changes in Enhancement/566 Confirm Classification before Save #609) but no longer seeing the Classify button if the feature is disabled on the post level:
Yeah, or just make that change in this PR, either way works for me |
Thanks for the feedback. Changes are implemented.
I have found that this is because the post status is being So as a fix, we can allow |
I was testing with an already published item so I don't think that's the issue I was facing. In looking closer, it seems we have a conditional in our |
Description of the Change
Closes #577
How to test the Change
Changelog Entry
Credits
Props @timatron @dkotter @faisal-alvi
Checklist: